Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Ingest Manager] Remove fields from index pattern during package uninstall #80082

Merged

Conversation

jonathan-buttner
Copy link
Contributor

@jonathan-buttner jonathan-buttner commented Oct 8, 2020

Summary

This PR addresses this issue: #80031

The installIndexPatterns() function relies on the saved objects being absent for removing the fields from a package during uninstall. Since the saved object is still present when the function is called it will collection the same fields as before instead of removing them fields for the package being uninstalled.

Testing

  1. Start a fresh kibana and elasticsearch
  2. Navigate to the ingest manager page to trigger /setup
  3. Install the crowdstrike 0.1.1 package
curl --location --request POST 'http://elastic:changeme@localhost:5601/api/fleet/epm/packages/crowdstrike-0.1.1' \
--header 'Accept: application/json' \
--header 'Content-Type: application/json' \
--header 'kbn-xsrf: xxx' \
--data-raw ''
  1. Navigate back to the index pattern management page and observe that the crowdstrike fields exist
    image

  2. Uninstall the crowdstrike package

curl --location --request DELETE 'http://elastic:changeme@localhost:5601/api/fleet/epm/packages/-0.1.1' \
--header 'Accept: application/json' \
--header 'Content-Type: application/json' \
--header 'kbn-xsrf: xxx' \
--data-raw ''
  1. Navigate back to the index pattern management page and observe that the crowdstrike fields are removed.

image

@jonathan-buttner jonathan-buttner added release_note:fix v8.0.0 Feature:EPM Fleet team's Elastic Package Manager (aka Integrations) project Team:Fleet Team label for Observability Data Collection Fleet team v7.11.0 labels Oct 8, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ingest-management (Team:Ingest Management)

@elasticmachine
Copy link
Contributor

Pinging @elastic/ingest-management (Feature:EPM)

@jonathan-buttner
Copy link
Contributor Author

@ph should we backport this so it's fixed in 7.10?

@@ -200,6 +203,7 @@ export default function (providerContext: FtrProviderContext) {
describe('uninstalls all assets when uninstalling a package', async () => {
skipIfNoDockerRegistry(providerContext);
before(async () => {
await installPackage(pkgKey);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it adds then immediately removes the package. Is that what's happening?

Copy link
Contributor Author

@jonathan-buttner jonathan-buttner Oct 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah this is just ensuring that the package was installed to begin with so the fields get added and then immediately removes it to setup the tests to validate the uninstall scenario. I got in a weird state locally when running the tests where the package was never actually installed the first time.

@@ -324,6 +328,35 @@ export default function (providerContext: FtrProviderContext) {
}
expect(resSearch.response.data.statusCode).equal(404);
});
it('should have removed the fields from the index patterns', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused about which branch we expect to execute. The try or the catch?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both could happen. If a test case in another file calls /setup then the system and endpoint packages will be installed and will be present for the remainder of the tests (because they cannot be removed). If that is the case the expect in the try will work because the logs-* and metrics-* index patterns will still be present even after this test uninstalls its package.

If /setup was never called prior to this test, when the package is uninstalled the index pattern code checks to see if there are no packages installed and completely removes the logs-* and metrics-* index patterns. If that happens this code will throw an error and indicate that the index pattern being searched for was completely removed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, interesting. Maybe we could add that as a comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah sure I'll put it as a comment.

Copy link
Contributor

@skh skh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, 👍

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@jonathan-buttner jonathan-buttner merged commit bc6ba87 into elastic:master Oct 12, 2020
@jonathan-buttner jonathan-buttner deleted the fix-index-pattern-deletion branch October 12, 2020 17:19
jonathan-buttner added a commit to jonathan-buttner/kibana that referenced this pull request Oct 12, 2020
…stall (elastic#80082)

* Correctly removing index pattern fields

* Adding comment about try/catch
jonathan-buttner added a commit to jonathan-buttner/kibana that referenced this pull request Oct 12, 2020
…stall (elastic#80082)

* Correctly removing index pattern fields

* Adding comment about try/catch
jonathan-buttner added a commit that referenced this pull request Oct 12, 2020
…stall (#80082) (#80195)

* Correctly removing index pattern fields

* Adding comment about try/catch
jonathan-buttner added a commit that referenced this pull request Oct 12, 2020
…stall (#80082) (#80194)

* Correctly removing index pattern fields

* Adding comment about try/catch
jloleysens added a commit to jloleysens/kibana that referenced this pull request Oct 13, 2020
…a-tier-migration

* 'master' of github.com:elastic/kibana: (34 commits)
  Line Visualization improper scaling can result in gaps elastic#79663 (elastic#80135)
  [Security Solution][Timeline] Fix SelectableTimeline search (elastic#80128)
  move field_mapping util to saved_objects plugin (elastic#79918)
  [ML] Datagrid: Ensure column content with 'boolean' schema is not capitalized (elastic#80041)
  [CI] Correctly resolve repository root for JUnit reports (elastic#80226)
  [Ingest Manager] Fix package upgrade breaking after first rollover before new data has arrived (elastic#79887)
  [Security Solution] Fix positioning of Kibana banners list inside Sec… (elastic#80124)
  add missing await to fix test (elastic#80202)
  Revert test data changed in previous commit. (elastic#79479)
  [Security Solution] [Sourcerer] Jest beef up (elastic#79907)
  Re-enable transaction duration alert story (elastic#80187)
  [npm] remove canvas dep (elastic#80185)
  [DOCS] Redirects ILM docs to Elasticsearch reference (elastic#79798)
  [APM] Catch health status error from ML (elastic#80131)
  Fix layout and remove title for add alert popover. (elastic#77633)
  [Discover] Loading spinner cleanup (elastic#79819)
  [Security Solution] [Resolver] Remove related events api (elastic#79036)
  [Ingest Manager] Remove fields from index pattern during package uninstall (elastic#80082)
  do not refetch license if signature header absents from a response (elastic#79645)
  Only send agent data for non-opentelemetry agents (elastic#79587)
  ...

# Conflicts:
#	x-pack/plugins/index_lifecycle_management/server/routes/api/nodes/register_list_route.ts
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 13, 2020
* upstream/master: (34 commits)
  Improve vis editor typings (elastic#80004)
  Line Visualization improper scaling can result in gaps elastic#79663 (elastic#80135)
  [Security Solution][Timeline] Fix SelectableTimeline search (elastic#80128)
  move field_mapping util to saved_objects plugin (elastic#79918)
  [ML] Datagrid: Ensure column content with 'boolean' schema is not capitalized (elastic#80041)
  [CI] Correctly resolve repository root for JUnit reports (elastic#80226)
  [Ingest Manager] Fix package upgrade breaking after first rollover before new data has arrived (elastic#79887)
  [Security Solution] Fix positioning of Kibana banners list inside Sec… (elastic#80124)
  add missing await to fix test (elastic#80202)
  Revert test data changed in previous commit. (elastic#79479)
  [Security Solution] [Sourcerer] Jest beef up (elastic#79907)
  Re-enable transaction duration alert story (elastic#80187)
  [npm] remove canvas dep (elastic#80185)
  [DOCS] Redirects ILM docs to Elasticsearch reference (elastic#79798)
  [APM] Catch health status error from ML (elastic#80131)
  Fix layout and remove title for add alert popover. (elastic#77633)
  [Discover] Loading spinner cleanup (elastic#79819)
  [Security Solution] [Resolver] Remove related events api (elastic#79036)
  [Ingest Manager] Remove fields from index pattern during package uninstall (elastic#80082)
  do not refetch license if signature header absents from a response (elastic#79645)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:EPM Fleet team's Elastic Package Manager (aka Integrations) project release_note:fix Team:Fleet Team label for Observability Data Collection Fleet team v7.10.0 v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants